Skip to content

Fix #671: Missing newline at end of file should produce a warning#672

Open
glankk wants to merge 3 commits into
cppcheck-opensource:masterfrom
glankk:671
Open

Fix #671: Missing newline at end of file should produce a warning#672
glankk wants to merge 3 commits into
cppcheck-opensource:masterfrom
glankk:671

Conversation

@glankk

@glankk glankk commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

@firewave firewave linked an issue Jul 9, 2026 that may be closed by this pull request
@firewave

firewave commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

All these stray -1 and \n are really awful and make this feel very wrong.

@danmar

danmar commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

All these stray -1 and \n are really awful and make this feel very wrong.

I am not sure why the -1 are needed. Not saying it's wrong, I just don't understand it.

The \n makes the testcode technically valid. Are you suggestion to remove those and ensure that warnings are written properly?

@firewave

firewave commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

I am not sure why the -1 are needed. Not saying it's wrong, I just don't understand it.

Me neither. Passing less means that the newline would be missing and the other changes add it. That is kind of a flip-flop.

The \n makes the testcode technically valid. Are you suggestion to remove those and ensure that warnings are written properly?

If fiddling with the trailing newline is necessary that seems like a possible source of issues for users of the library.

@glankk

glankk commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

I am not sure why the -1 are needed. Not saying it's wrong, I just don't understand it.

The char arrays that are initialized with a string constant include a null terminator, which makes these test cases fail because they end with '\0' instead of '\n'.

All these stray -1 and \n are really awful and make this feel very wrong.

I agree that it looks really ugly but it's not wrong.

@glankk

glankk commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

I also agree that it's a bit awkward to implement this in simplecpp, since it doesn't really distinguish between C and C++.

@firewave

Copy link
Copy Markdown
Collaborator

I am not sure why the -1 are needed. Not saying it's wrong, I just don't understand it.

The char arrays that are initialized with a string constant include a null terminator, which makes these test cases fail because they end with '\0' instead of '\n'.

All these stray -1 and \n are really awful and make this feel very wrong.

I agree that it looks really ugly but it's not wrong.

Maybe we should something more explicit to highlight what we are doing and it doesn't look like we randomly added those. We should probably land #666 first which allows passing the actual sizes instead of just relying onto strlen().

@danmar

danmar commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator

If fiddling with the trailing newline is necessary that seems like a possible source of issues for users of the library.

Of course we should write test code properly. If the newline is not included in the test code then it is not included.
Let's write test code exactly as we want it to be.

All these stray -1 and \n are really awful and make this feel very wrong.
I agree that it looks really ugly but it's not wrong.

It's not "ugly" to write test code properly.

I understand now why you write sizeof(code)-1 and that is the proper way to calculate the size.

I.e. if we have const char code[] = "abcde"; then sizeof(code) will return 6. If we pass 6 as size argument to the preprocess function then we tell the preprocess function that it should read the null character after the 'e' when preprocessing. If we don't intended that then we should not pass 6.

@danmar

danmar commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator

I also agree that it's a bit awkward to implement this in simplecpp, since it doesn't really distinguish between C and C++.

I want that we are able to find this UB. But we do need to distinguish C/C++ somewhere. I can see two options:

  • add a flag for TokenList so that it knows if it is C code. Maybe as a parameter to the constructors.
  • instead of emitting NO_EOF_NEWLINE, we could have a Location list in TokenList called missingNewline. Tools can inspect that list after preprocessing to determine if they should write warnings.
  • current solution: simplecpp will emit the NO_EOF_NEWLINE portability warning, and the tool must determine what it should do about that. I.e. Cppcheck will just it for C++ files.

I think current solution sounds least ugly.

Comment thread main.cpp
std::cerr << "portability: ";
break;
case simplecpp::Output::PORTABILITY_NO_EOF_NEWLINE:
std::cerr << "portability: ";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could adjust main.cpp so that this output is only written if the filename extension is ".c".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missing newline at end of file should produce a warning

3 participants